-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix error caused by updating url too frequently #726
Conversation
✅ Deploy Preview for veda-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
3ee7362
to
881fb48
Compare
This stared as a PR to fix the fact that the URL was being updated too frequently, but it turned out that there was another problem related to the URL. I added a README with an explanation of both problems and what was done to solve them. I feel that this whole url business is a bit hack-y especially Problem 2. We're basically caching values in between state updates which feels a bit wrong. It would be great to have a second opinion on this. @hanbyul-here @sandrahoang686 . If you have ideas for different approached let's discuss. |
I was hoping to try the suggestion on my end first and leave a comment, but it looks like I won't have the luxury of it, so I will just comment 😢 Have you considered using Jotai's custom compare function? https://jotai.org/docs/recipes/atom-with-compare ? We would need to pass deepcompare function in the end, but can take a bit of advantage of Jotai system. |
@hanbyul-here Thank you for the tip. Even though I was not able to use Thank you 😊 . Have a look if you're able |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
atom with url value stability makes more sense with your change, thanks for working on it @danielfdsilva
This is not a blocker, but I can't help wonder if we need to update the atom value on drag event. I wonder if we can just update on the release? (Apologies if this was already discussed before!)
We do need to store the value on drag so that the handle can move. We could store it in a state hook, and then update the atom, but since the atom is the state, I fear we'd be duplicating efforts. |
If the url is updated too often in a short period of time, the app crashes.
It also fixes the error where the analysis was always shown as outdated